feat: add opt-in crash reporting with MetricKit (#472) by batonogov · Pull Request #629 · batonogov/pine

batonogov · 2026-03-29T10:40:15Z

Closes #472 - MetricKit + signal handler fallback, opt-in dialog, menu toggle, 32 tests, 9 languages

github-actions · 2026-03-29T11:13:54Z

✅ Code Coverage: 71.4%

Threshold: 70%

Coverage is above the minimum threshold.

Generated by CI — see job summary for detailed file-level breakdown.

…llback (#472) - MetricKit (MXCrashDiagnostic) as primary crash diagnostic source - POSIX signal handler fallback (strictly async-signal-safe: only write/_exit) - Opt-in dialog on first launch with privacy explanation - Toggle in File menu to enable/disable at any time - CrashReport model with Codable serialization - CrashReportStore persists reports to Application Support - Localization for all 9 languages (de, en, es, fr, ja, ko, pt-BR, ru, zh-Hans) - 32 unit tests covering settings, model, store, parseCallStack, edge cases

…action, JSON callstack parsing

batonogov · 2026-03-31T03:29:11Z

Code Review: feat: add opt-in crash reporting with MetricKit (#472)

Вердикт: REQUEST CHANGES (не могу поставить формально, т.к. свой PR)

Билд и SwiftLint прошли. Архитектура в целом грамотная: разделение на Settings/Store/Manager/View правильное, signal handler async-signal-safe, opt-in flow на месте. Но есть критические проблемы.

CRITICAL — Обязательно исправить

1. Данные НЕ отправляются, но UI говорит пользователю что они отправляются

Локализованные строки во всех 9 языках говорят: "Pine can send anonymous crash reports". Но в коде нет НИКАКОЙ сетевой отправки — ни URLSession, ни endpoint, ни API-вызовов. Crash reports складываются в ~/Library/Application Support/Pine/CrashReports/ и там лежат.

Это либо:

(a) Обман пользователя — обещаем отправку, ничего не отправляем
(b) Незавершённая фича — забыли реализовать отправку

В обоих случаях нельзя мержить. Либо убрать слово "send" из всех локализаций и переформулировать как "Pine can collect crash reports locally to help diagnose issues", либо реализовать отправку.

2. CrashReportStore — не thread-safe

CrashReportStore — обычный final class, save() вызывается из processCrashDiagnostic() (MetricKit callback приходит на произвольном потоке), checkForCrashMarker() вызывается с main. Одновременный save() + loadAll() + pruneOldReports() — data race. Нужен DispatchQueue или actor.

3. CrashReportingManager не помечен @MainActor, но работает с UI-state

startIfEnabled() вызывается из:

applicationDidFinishLaunching (main thread)
.onChange(of: crashReportingEnabled) (SwiftUI, main thread)
Opt-in callback в sheet (main thread)

Но didReceive(_:) (MXMetricManagerSubscriber) вызывается MetricKit на произвольном потоке. Внутри он дёргает store.save(), который не thread-safe (см. выше). Класс наследует NSObject — не получится сделать actor, но нужна внутренняя синхронизация.

4. Signal handler — memory leak strdup

let cString = strdup(path)
crashMarkerCString = cString

strdup выделяет память через malloc, которая никогда не освобождается. Если installSignalHandlers() вызовется повторно (toggle on/off/on), будет утечка при каждом вызове. stop() не очищает signal handlers и не освобождает crashMarkerCString. Нужна защита от повторного вызова или free() старого значения.

IMPORTANT — Нужно исправить

5. stop() не восстанавливает signal handlers

stop() только делает MXMetricManager.shared.remove(self), но signal handlers остаются установленными. После stop() краш всё равно запишет marker file. Нужно восстанавливать SIG_DFL для всех сигналов.

6. Opt-in dialog показывается в КАЖДОМ окне проекта

showCrashReportingOptInIfNeeded() вызывается в ContentView.onAppear. Если у пользователя открыто 3 проекта, диалог покажется 3 раза (race condition — hasShownPrompt ставится только по клику, а не по показу). Нужен @MainActor-изолированный флаг или проверка прямо перед показом sheet.

7. CrashReportingSettings — статические свойства на UserDefaults.standard без синхронизации

needsPrompt читает hasShownPrompt, а recordChoice() пишет его. Между чтением и записью в разных окнах может быть race. Нужен хотя бы атомарный check-and-set.

8. Нет UI для просмотра/экспорта crash reports

Crash reports складываются на диск, но пользователь не может их увидеть, экспортировать или удалить через UI. Issue #472 предлагал "send to GitHub Issues automatically or via email". Текущая реализация — мёртвое хранилище данных. Как минимум нужен пункт меню "View Crash Reports" или "Export Crash Report".

9. Нет теста на pruning edge case

Тест pruning_removesOldReportsOverLimit сохраняет maxReports + 5 элементов, но pruning вызывается после КАЖДОГО save(), поэтому count никогда не превысит maxReports + 1. Тест проходит, но не проверяет реальный сценарий. Что не проверено: что при pruning удаляются именно СТАРЫЕ, а не новые.

10. Локализация opt-in кнопок — hardcoded String(localized:) вместо Strings.swift

В CrashReportingOptInView.swift:

Button(String(localized: "crashReporting.optIn.disable")) { ... }
Button(String(localized: "crashReporting.optIn.enable")) { ... }

Остальные строки идут через Strings.swift, а кнопки — напрямую. Нарушение конвенции проекта. Перенести в Strings.swift.

SUGGESTIONS — Желательно

11. Иконка ant.circle не подходит семантически

Для crash reporting лучше exclamationmark.triangle или ladybug (Apple HIG diagnostics) или stethoscope.

12. CrashReport.parseCallStack — тривиальная реализация

Просто split по newlines + trim. Название misleading — это не "parsing", а "splitting". Либо переименовать в splitCallStackLines, либо добавить реальный парсинг.

13. Privacy note говорит "open tab count", но openTabCount всегда nil

В processCrashDiagnostic() openTabCount не заполняется. MetricKit callback не имеет доступа к UI state. Либо убрать из privacy note, либо реализовать.

Итого

4 критических, 6 важных, 3 предложения. Главная проблема: фича обещает пользователю отправку crash reports, но ничего не отправляет. Thread safety отсутствует в Store и Manager. Отправить на доработку.

…text, race condition - CrashReportStore: add serial DispatchQueue for all disk I/O (thread safety) - CrashReportStore: add revealInFinder() and copyAllToClipboard() export methods - CrashReportingManager.stop(): restore default signal handlers + free strdup'd C string to prevent memory leak on toggle cycles - CrashReportingManager.installSignalHandlers(): free previous strdup on re-install - ContentView+Helpers: set hasShownPrompt immediately before async delay to prevent opt-in dialog showing in multiple windows simultaneously - CrashReportingOptInView: use Strings constants instead of inline String(localized:) - Localizable.xcstrings: replace misleading "send" language with "collect locally" / "stored locally" across all 9 locales (en, de, es, fr, ja, ko, pt-BR, ru, zh-Hans) - Strings.swift: add crashReportingOptInEnable/Disable constants - Tests: add thread safety, export, race condition, stop(), and strings tests

batonogov

Повторное ревью PR #629

Статус предыдущих замечаний

UI обман "send" — ИСПРАВЛЕНО. Тексты теперь "collect locally" / "stored locally". Методы revealInFinder() и copyAllToClipboard() реализованы в CrashReportStore.
Thread safety CrashReportStore — ИСПРАВЛЕНО. Serial DispatchQueue, все публичные методы через queue.sync.
Memory leak в signal handler — ИСПРАВЛЕНО. stop() вызывает free() + SIG_DFL. installSignalHandlers() освобождает предыдущий strdup.
Opt-in dialog race — ИСПРАВЛЕНО. hasShownPrompt = true стоит ДО asyncAfter.
Hardcoded strings — ИСПРАВЛЕНО. Все строки в Strings.swift.
"send" в Localizable.xcstrings — ИСПРАВЛЕНО.

Новые проблемы

Critical (must fix)

C1. SwiftLint failure — CI красный.
PineTests/CrashReportingTests.swift:646 — empty_count violation. Переменная count сравнивается с 0:

let count = store.copyAllToClipboard()
#expect(count == 0)

SwiftLint трактует это как нарушение. Переименовать переменную:

let copiedCount = store.copyAllToClipboard()
#expect(copiedCount == 0)

Аналогично проверить остальные вхождения count == / count != в этом файле.

C2. revealInFinder() и copyAllToClipboard() — мёртвый код.
Эти методы реализованы в CrashReportStore, но НЕ подключены ни к какому UI-элементу. Пользователь не может до них добраться. Замечание #1 требовало добавить их в UI (меню или кнопки). Сейчас это мёртвый код — нужно добавить пункты меню в PineApp.swift (например в секцию File или рядом с toggle crash reporting), либо добавить кнопки в CrashReportingOptInView/Settings.

Important (should fix)

I1. Signal handler использует Swift runtime.
signalHandler() — Swift function, использующая guard let, withUnsafePointer, var/let — всё это может вызывать Swift runtime, что не является async-signal-safe. Для fallback-маркера это допустимый компромисс, но стоит как минимум добавить @convention(c) аннотацию и комментарий, что это best-effort fallback (а не строго async-signal-safe).

I2. CrashReportingManager не имеет thread safety гарантий.
startIfEnabled() и stop() обращаются к глобальной crashMarkerCString без синхронизации. На практике оба вызываются с main thread, но это нигде не задокументировано и не enforced. Добавить @MainActor на CrashReportingManager или хотя бы dispatchPrecondition(condition: .onQueue(.main)) в start/stop.

Suggestions (nice to have)

S1. CrashReportStore.save() использует queue.sync — может заблокировать вызывающий поток.
Если save() вызывается из MetricKit callback (произвольный поток), queue.sync блокирует этот поток до завершения I/O. Лучше queue.async для save() — fire and forget, ведь результат не нужен вызывающему.

S2. Тесты withCleanDefaults дублируют код.
CrashReportingSettingsTests и CrashReportingSettingsRaceTests содержат идентичный withCleanDefaults(). Вынести в общий helper.

…, SwiftLint - Rename count to copiedCount in tests to fix SwiftLint empty_count - Add menu buttons for Reveal Crash Reports in Finder and Copy Crash Reports so revealInFinder()/copyAllToClipboard() are no longer dead code - Replace bare crashMarkerCString global with CrashMarkerStorage struct using os_unfair_lock for thread-safe access (trylock in signal handler) - Rewrite signal handler to avoid Swift runtime: no guard/let unwrap, no withUnsafePointer closures, byte-by-byte POSIX write() - Add tests for new menu icons

batonogov · 2026-03-31T03:56:39Z

Code Review — Раунд 3

Все 4 замечания раунда 2 исправлены ✓

Новые проблемы

Important:

.disabled(CrashReportStore.shared.loadAll().isEmpty) — disk I/O на main thread при каждом рендере меню (PineApp.swift). loadAll() читает директорию + десериализует до 50 JSON файлов. Используй CrashReportStore.shared.count > 0 или кэшируй count.
Неполная локализация — menu.crashReports.copy и menu.crashReports.reveal имеют только en/ru, а остальные новые строки — 9 языков. Добавь недостающие 7 переводов.

Suggestions:

Комментарий в signal handler "No Swift runtime (no guard/let/Optional unwrap)" противоречит guard let в writeMarkerIfPossible. Исправь комментарий.
strdup(path) может вернуть nil при OOM — стоит залогировать warning.

…r docs - Replace loadAll().isEmpty with lightweight isEmpty property (dir listing only, no JSON decoding) to avoid disk I/O on every menu render - Add missing 7 translations (de, es, fr, ja, ko, pt-BR, zh-Hans) for menu.crashReports.copy and menu.crashReports.reveal in xcstrings - Fix misleading signal handler comment — guard let on raw pointer is safe - Add warning log when strdup returns nil (OOM) - Add isEmpty test for CrashReportStore

batonogov · 2026-03-31T04:06:27Z

Code Review — Раунд 4

Все 4 замечания раунда 3 исправлены ✓

Новая проблема

Important: Toggle через меню в PineApp.swift (~line 457) не ставит hasShownPrompt = true. Если пользователь включит crash reporting через menu toggle (а не через opt-in диалог), при следующем запуске opt-in диалог покажется повторно.

Fix: одна строка CrashReportingSettings.hasShownPrompt = true в onChange handler.

…472-update

…atibility

batonogov · 2026-03-31T07:49:55Z

Code Review -- Round 5 (final)

Прошёлся по полному diff (12 файлов, ~2100 строк). Все замечания с раундов 1-4 исправлены.

Что сделано хорошо

Signal handler строго async-signal-safe: stack buffer для цифр, побайтовый write(), никакого Swift runtime
CrashMarkerStorage с os_unfair_lock + trylock в signal handler -- корректный паттерн
CrashReportStore -- serial queue для всех операций, isEmpty/count через directory listing без JSON-декодирования
Opt-in dialog: hasShownPrompt = true ДО async delay -- race condition между окнами закрыт
stop() восстанавливает SIG_DFL + освобождает strdup'd строку
840 строк тестов: settings, model, store, callstack parsing, thread safety, export, race conditions, stop(), strings, menu icons
Swift 6 fix: замена signal(sig, signalHandler) на closure -- правильно
Локализация 9 языков, xcstrings в правильном формате (targeted insertion)

Critical / Important

Нет. Все ранее найденные проблемы исправлены.

Итого

PR готов к мерджу (после прохождения CI).

batonogov added 2 commits March 31, 2026 05:48

fix: async-signal-safe handler with pre-computed C string, proper sig…

4af84a3

…action, JSON callstack parsing

batonogov force-pushed the feat/crash-reporting-472 branch from f467098 to 4af84a3 Compare March 31, 2026 02:51

batonogov added enhancement New feature or request priority: medium Medium priority labels Mar 31, 2026

Merge remote-tracking branch 'origin/main' into feat/crash-reporting-472

7957aa7

batonogov commented Mar 31, 2026

View reviewed changes

batonogov added 3 commits March 31, 2026 07:22

fix: set hasShownPrompt when toggling crash reporting via menu

69e3933

Merge remote-tracking branch 'origin/main' into feat/crash-reporting-…

2465535

…472-update

fix: wrap signalHandler in literal closure for Swift 6 C pointer comp…

2c03b0a

…atibility

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add opt-in crash reporting with MetricKit (#472)#629

feat: add opt-in crash reporting with MetricKit (#472)#629
batonogov wants to merge 9 commits intomainfrom
feat/crash-reporting-472

batonogov commented Mar 29, 2026

Uh oh!

github-actions bot commented Mar 29, 2026 •

edited

Loading

Uh oh!

batonogov commented Mar 31, 2026

Uh oh!

batonogov left a comment

Uh oh!

batonogov commented Mar 31, 2026

Uh oh!

batonogov commented Mar 31, 2026

Uh oh!

batonogov commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

batonogov commented Mar 29, 2026

Uh oh!

github-actions bot commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Code Coverage: 71.4%

Uh oh!

batonogov commented Mar 31, 2026

Code Review: feat: add opt-in crash reporting with MetricKit (#472)

CRITICAL — Обязательно исправить

IMPORTANT — Нужно исправить

SUGGESTIONS — Желательно

Итого

Uh oh!

batonogov left a comment

Choose a reason for hiding this comment

Повторное ревью PR #629

Статус предыдущих замечаний

Новые проблемы

Critical (must fix)

Important (should fix)

Suggestions (nice to have)

Uh oh!

batonogov commented Mar 31, 2026

Code Review — Раунд 3

Новые проблемы

Uh oh!

batonogov commented Mar 31, 2026

Code Review — Раунд 4

Новая проблема

Uh oh!

batonogov commented Mar 31, 2026

Code Review -- Round 5 (final)

Что сделано хорошо

Critical / Important

Итого

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

github-actions bot commented Mar 29, 2026 •

edited

Loading